/ocabot migration queue_job

/ocabot migration test_queue_job

@hoangtrann quick update:

• We’re down to just 3 open comments on this PR; the rest are addressed and pushed.
• When you have a moment, a fresh check would be super helpful.

I’d also really appreciate it if you could take a look at PR #842—specifically the tests:

You can run the suite locally with (use any throwaway DB name in place of <DB_NAME>):

./odoo/odoo-bin -c odoo.conf -d <DB_NAME>
--init queue_job,queue_job_batch,queue_job_cron,queue_job_cron_jobrunner,queue_job_subscribe,test_queue_job,test_queue_job_batch
--test-tags="/queue_job,/queue_job_batch,/queue_job_cron,/queue_job_cron_jobrunner,/queue_job_subscribe,/test_queue_job,/test_queue_job_batch"
--stop-after-init

Thanks again for the thorough and detailed review—really grateful for your contributions and feedback 🙏

there are still some comments stating odoo 19 scattered across the changes which I thing we should removed.

for anyone comes after these are removed, I think it's good to just summarized the major changes to the description of the pr which can also works as a good resource for reference in other mig pr

Resolved in: d27f83d

From my checks and tests, this is all ready to go. Can it be merged?

@sbidoul

@tishmen have you tested this version with the preforkserver configuration (using --workers=4)? Jobs are enqueued, but never started.
The threaded server setup does work.

Additionally, there is an error due to the upstream config refactoring in this method:
https://github.com/tishmen/queue/blob/pr/queue_job_base/queue_job/jobrunner/runner.py#L474-L479. On the line db_names = config["db_name"].split(",") specifically: config["db_name"] is no longer a comma separated string, but already a list so the split can be omitted.

@tishmen An additional refactoring is necessary here because read_group is no longer supported by the core. You'll have to refactor this into the use of the new _read_group method.

@PieterPaulussen fyi, workers mode working fine here, is the runner starting properly on your end?

$ egrep "(runner|runner\.channels): .*(starting|initializing|.+runner ready|database connections ready|Configured channel)" odoo.log

@PieterPaulussen fyi, workers mode working fine here, is the runner starting properly on your end?

$ egrep "(runner|runner\.channels): .*(starting|initializing|.+runner ready|database connections ready|Configured channel)" odoo.log

@nilshamerlinck Curious, could you share your worker and channel configuration please? In my test, the jobs got enqueued, but were never actually processed.

@tishmen Please take a look at this commit. I would create a PR for your branch, but for some reason I couldn't do it.

It's a fix related to a change in db_name parsing from config (src)

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@pedrobaeza Sorry for tagging you here again but you are the only one that I know has merge rights. Would you mind merging this? Thanks in advance.

Hi, I'm afraid I don't know too much this module, so one PSC like @guewen, @simahawk or @sbidoul should check that it's correct and then merge.

Hi, I'm afraid I don't know too much this module, so one PSC like @guewen, @simahawk or @sbidoul should check that it's correct and then merge.

Thanks for tagging them.

I plan to read the latest comments and re-review soon(ish). Maybe next week.

It the meantime, nothing prevents using it, and reporting further issues if any.

@tishmen Well done - all I can really say is, that I'm glad my PR was superseded - 😜

Overall, I really think we should be getting these PRs for migrations through as efficiently as possible. I know everyone has their opinions, and where code has been touched or altered, it is great to aim for perfection, but if the changes are not wrong, if they are not regressive, then there should be no obligation to "improve" during a migration. Nice if it does, but it should not be obligatory.

After watching this unfold, I will be VERY uninclined to offer a module migration, as my objective will be to get the module across as is, and correct, not suddenly inheriting everyone else's wishlists...

Of course, this does not mean dismissing anyone's reviews or input without due and proper consideration, but the end game of a migration PR seems different for different people....

@sbidoul I will take care of all of the code review comments this weekend. Thanks for your feedback.

Здраво, @tishmen!

@sbidoul I will take care of all of the code review comments this weekend.

Hope you had nice weekend :D

Sorry for the delay — I’ve pushed updates addressing the review feedback (style nits, test cleanups, and the _read_group perf tweak). Pre-commit is green again. Please take another look when you have a moment.

@sbidoul can we help somehow on this?

Just a question, there is need for so many migration commits or should it be squashed in a single one?

About the commits, this helped during review but I suppose it's best to squash them now. Then we can merge.

Hi all! I’ve now squashed PR #840 down to a single commit as requested, and kept the latest review fixes in place. Pre-commit and an Odoo --stop-after-init install check are green on my side. Could you please re-review when you have a moment? @sbidoul @etobella

Thanks for all this work @tishmen !

/ocabot merge nobump

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 19.0-ocabot-merge-pr-840-by-sbidoul-bump-nobump, awaiting test results.

Congratulations, your PR was merged at 1f23acd. Thanks a lot for contributing to OCA. ❤️

@sbidoul Thanks! And also huge thanks to everyone who reviewed, commented, and helped move this forward 🙏

See also #872 for the latest improvements from 18. Also we should replace the autovacuum cron with a @api.autovacuum decorator (with a migration to remove the cron). If anyone feels like doing this :)

Thanks for the pointer! 👍@sbidoul

I’ll take care of this in a separate PR this weekend (switching to the @api.autovacuum decorator and adding the migration to remove the cron).

Also, it would be greatly appreciated if the remaining OCA Queue–related PRs could get some review time as well. I really like to get them ready for merging.

📣 Call for help: if anyone reading this has some bandwidth to review or test the other Queue PRs, help would be very welcome. Thanks everyone!

PR Links:

#841
#842
#843
#844
#845